Refactoring interpreter and paranoid mode, introducing traits to allo… #15350
Conversation
…w generic interpreter as well as compile time switch for the runtime type checks (formerly called paranoid mode)
⏱️ 31m total CI duration on this PR
|
ziaptos
requested review from
georgemitenkov,
gelash,
runtian-zhou,
vgao1996 and
wrwg
| @@ -72,7 +72,7 @@ pub(crate) fn trace( | |||
| pc: u16, | |||
| instr: &Bytecode, | |||
| resolver: &Resolver, | |||
| interp: &Interpreter, | |||
| interp: &InterpreterImpl, | |||
There was a problem hiding this comment.
nit: interp --> interpreter
| struct_variant_instantiation_type: | ||
| BTreeMap<StructVariantInstantiationIndex, (Type, NumTypeNodes)>, | ||
| /// For a given field instantiation, the: | ||
| /// ((Type of the field, size of the field type) and (Type of its defining struct, size of its defining struct) |
There was a problem hiding this comment.
line too long, break at 100?
| /// ((Type of the field, size of the field type) and (Type of its defining struct, size of its defining struct) | ||
| field_instantiation: | ||
| BTreeMap<FieldInstantiationIndex, ((Type, NumTypeNodes), (Type, NumTypeNodes))>, | ||
| /// Same as above, bot for variant field instantiations |
| } | ||
|
|
||
| #[inline(always)] | ||
| pub(crate) fn get_field_type_and_struct_type( |
There was a problem hiding this comment.
These are just copy-paste, right?
| Ok(((field_ty, *field_ty_count), (struct_ty, *struct_ty_count))) | ||
| } | ||
|
|
||
| pub(crate) fn get_variant_field_type_and_struct_type( |
There was a problem hiding this comment.
For consistency, #[inline(always)]?😄
| operand_stack.push_ty(output_ty) | ||
| } | ||
|
|
||
| pub(crate) struct NullRuntimeTypeCheck; |
There was a problem hiding this comment.
See some C++ here ahah, NoRuntimeTypeCheck maybe?
|
|
||
| /// Paranoid type checks to perform after instruction execution. | ||
| /// | ||
| /// This function and `pre_execution_type_stack_transition` should constitute the full type stack transition for the paranoid mode. |
There was a problem hiding this comment.
I think lines are > 100 chars again?
| | Bytecode::Call(_) | ||
| | Bytecode::CallGeneric(_) | ||
| | Bytecode::Abort => { | ||
| // Invariants hold because all of the instructions above will force VM to break from the interpreter loop and thus not hit this code path. |
There was a problem hiding this comment.
again, multiple lines
| ) -> PartialVMResult<()>; | ||
| } | ||
|
|
||
| /// Paranoid type checks to perform before instruction execution. |
There was a problem hiding this comment.
If this is meant for the module, you can do //! Your description here on top of the file? This also seems like a good documentation to add to trait definition?
| .into_iter() | ||
| .zip(field_tys) | ||
| { | ||
| // Fields ability should be a subset of the struct ability because abilities can be weakened but not the other direction. |
|
|
||
| /// Main loop for the execution of a function. | ||
| /// | ||
| /// This function sets up a `Frame` and calls `execute_code_unit` to execute code of the | ||
| /// function represented by the frame. Control comes back to this function on return or | ||
| /// on call. When that happens the frame is changes to a new one (call) or to the one | ||
| /// at the top of the stack (return). If the call stack is empty execution is completed. | ||
| fn execute_main( | ||
| fn execute_main<RTTCheck: RuntimeTypeCheck>( |
There was a problem hiding this comment.
Love the generic bound name :)
| @@ -1147,7 +1204,7 @@ const CALL_STACK_SIZE_LIMIT: usize = 1024; | |||
| pub(crate) const ACCESS_STACK_SIZE_LIMIT: usize = 256; | |||
|
|
|||
| /// The operand stack. | |||
There was a problem hiding this comment.
nit: "operand and type"?
| } | ||
|
|
||
| impl InterpreterDebugInterface for InterpreterImpl { | ||
| #[allow(dead_code)] |
There was a problem hiding this comment.
Can we use some cfg(any(...)) or so? Seems like this is only used if some cfg feature is on
| active_modules: HashSet::new(), | ||
| }; | ||
|
|
||
| if loader.vm_config().paranoid_type_checks { |
There was a problem hiding this comment.
This is beautiful! One thing to note: we also have some paranoid checks inside execution loop (e.g., for functions). These can also be moved to this generic, but maybe you have a separate PR for that?
| @@ -57,9 +57,21 @@ macro_rules! set_err_info { | |||
| /// | |||
| /// An `Interpreter` instance is a stand alone execution context for a function. | |||
| /// It mimics execution on a single thread, with an call stack and an operand stack. | |||
| pub(crate) struct Interpreter { | |||
| pub(crate) struct Interpreter; | |||
There was a problem hiding this comment.
Where is this used? I see only Impl
Description
Refactoring interpreter and paranoid mode, introducing traits to allow generic interpreter as well as compile time switch for the runtime type checks (formerly called paranoid mode)
How Has This Been Tested?
Existing tests
Key Areas to Review
Should focus on making sure that the high level logic has not changed!
Type of Change
Which Components or Systems Does This Change Impact?
Checklist